Shouldn't the default be true? @tabowling what do you think?

Would it be easy to set safe_mode individually per pool or volume?
My reasoning is that one may need to reformat a disk. One then needs to set storage_safe_mode=false globally, and if one is managing the complete storage configuration in one call to the role, a mistake in an unrelated part of the configuration variable can then have unintended destructive consequences. It would be then safer to specify safe_mode only on the disks one needs to reformat.
(The tricky part is how to specify safe_mode for physical disks, which are provided in the "disks" arrays.)

How is reformatting a disk different from removing a file system in terms of being "unsafe" or "destructive"? Perhaps you are talking about formatting an unformatted disk, which would not be prevented by storage_safe_mode.

I am talking about formatting a disk which previously had some data on it (to be discarded on purpose). This should require storage_safe_mode=false. Setting it does then remove the protection against mistakes in other parts of the configuration.

I would say that this test is grossly insufficient considering the seriousness of the condition that it checks. What if the role destroys data and then sets blivet_output according to expectations?

A more thorough test is in #46

@dwlehman asserts will not do anything useful in a block that has ignore_errors: yes I am afraid
Also instead of ignoring errors, it would be better to actually check that the role fails at every invocation where it is supposed to, by catching the failure in rescue:, in addition to checking the output facts.

This does not work.

TASK [storage : Apply defaults to pools and volumes [2/6]] *********************
task path: /home/pcahyna/linux-system-roles/storage/tasks/main-blivet.yml:36
fatal: [/home/pcahyna/linux-system-roles-testing/local/rhel-guest-image-7.7-200.x86_64.qcow2]: FAILED! => {"msg": "could not find 'volumes' key in iterated item {'state': 'present', 'type': 'partition', 'name': 'foo', 'disks': ['vdc']}"}

It is arguably a bug that one can not create an empty pool (partition or other). But until this is fixed, tests should take it into account.

Moreover, even if I delete type: partition and add one volume in an effort to make it work, blivet refuses to create a VG on a disk which already contains a filesystem, even with safe mode off, so the test still fails:

TASK [storage : manage the pools and volumes to match the specified state] *****
task path: /home/pcahyna/linux-system-roles/storage/tasks/main-blivet.yml:102
fatal: [/home/pcahyna/linux-system-roles-testing/local/rhel-guest-image-7.7-200.x86_64.qcow2]: FAILED! => {"actions": [], "changed": false, "leaves": [], "mounts": [], "msg": "Failed to commit changes to disk", "packages": ["xfsprogs", "lvm2"], "pools": [], "volumes": []}

PR #46 has the change to create one volume, but this still fails as in the previous comment.

Moreover, even if I delete type: partition and add one volume in an effort to make it work, blivet refuses to create a VG on a disk which already contains a filesystem, even with safe mode off, so the test still fails:

TASK [storage : manage the pools and volumes to match the specified state] *****
task path: /home/pcahyna/linux-system-roles/storage/tasks/main-blivet.yml:102
fatal: [/home/pcahyna/linux-system-roles-testing/local/rhel-guest-image-7.7-200.x86_64.qcow2]: FAILED! => {"actions": [], "changed": false, "leaves": [], "mounts": [], "msg": "Failed to commit changes to disk", "packages": ["xfsprogs", "lvm2"], "pools": [], "volumes": []}

Sorry, I think it was because I forgot to specify size for the volume. The error message is very misleading though

It appears that type: partition is not implemented.

TASK [storage : get required packages] *****************************************
task path: /home/pcahyna/linux-system-roles/storage/tasks/main-blivet.yml:88
fatal: [/home/pcahyna/linux-system-roles-testing/local/rhel-guest-image-7.7-200.x86_64.qcow2]: FAILED! => {"actions": [], "changed": false, "leaves": [], "mounts": [], "msg": "Pool 'foo' has unknown type 'partition'", "packages": [], "pools": [], "volumes": []}

A fix for the test to not use partition for now is in #46

@dwlehman Why do we have type: disk in cleanup when we had type: partition in the previous task? Cleanup does not seem to happen properly.
Just replacing disk by partition does not work either though:
... Traceback (most recent call last):\r\n File \"/root/.ansible/tmp/ansible-tmp-1571753120.2970297-255844982294535/AnsiballZ_blivet.py\", line 114, in <module>\r\n _ansiballz_main()\r\n File \"/root/.ansible/tmp/ansible-tmp-1571753120.2970297-255844982294535/AnsiballZ_blivet.py\", line 106, in _ansiballz_main\r\n invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\r\n File \"/root/.ansible/tmp/ansible-tmp-1571753120.2970297-255844982294535/AnsiballZ_blivet.py\", line 49, in invoke_module\r\n imp.load_module('__main__', mod, module, MOD_DESC)\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 784, in <module>\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 781, in main\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 747, in run_module\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 568, in manage_volume\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 230, in manage\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 149, in _look_up_device\r\n File \"/tmp/ansible_blivet_payload_ADJzqt/__main__.py\", line 283, in _get_device_id\r\nAttributeError: 'NoneType' object has no attribute '_disks'\r\n", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1

Ah, maybe i have to change volumes to pools, partition is a pool type, not a volume type

It is both a pool type and a volume type, much like lvm.

@dwlehman but

          vars:
            storage_volumes:
              - name: test1
                type: partition

did not work, see the error above, I had to change to the version that is there now:

      vars:
        storage_pools:
          - name: foo
            type: partition
            disks: "{{ unused_disks }}"
            state: absent

Sorry, I didn't look closely enough at the context. There is indeed a 'partition' volume type but, just like the 'lvm' volume type, such volumes must be defined within a pool of the same type.

@dwlehman Should be foo, no? Not test1

Is there a present option? I would suppose that this would be handled via state (present or absent).

This commit da350a9 in PR #54 hopefully fixes the cleanup issues.

@dwlehman shouldn't we make it a separate test rather than asserting every time the role is used? This commit was intended to quickly illustrate the problem, but maybe it is not ideal to keep it in the long term.

Weird, I tried to leave that one out for that reason. Do you know which commit it came with? Maybe one of the merge commits?

Should be this:
Merge remote-tracking branch 'pcahyna/mount-assert' into non-destructive
57dd5cc

IMHO abbreviations like w/ should be avoided to keep this easier to read.

These are only the output of integration tests, but okay.

IMHO abbreviations like w/ should be avoided to keep this easier to read.

IMHO abbreviations like w/ should be avoided to keep this easier to read.

FYI: The lines are so long that GitHub requires horizontal scrolling to be able to read the full line. Seems like there is space for about 118 characters. Ideally the lines would be at most 88 characters IMHO.

This requirement is arbitrary at best and silly at worst. How did you arrive at 88 chars as a max? It was once 65 or 68, then it was 70-something, then 80, now 88? Most displays are 1200px or wider, so I don't understand the need to limit line lengths like we did when there was a fixed console width, but I also don't want to spend too much time arguing about it.

The 88 character suggestion comes from the black formatter https://github.com/psf/black#line-length - to me the arguments make sense. I do not want to argue about it, I just use black.

Regarding the display size - I have a 3840 px display, however as I wrote GitHub does not use all the space to show your long line but requires vertical scrolling. This makes it painful to read these lines. Also it is best-practice in the analog world to wrap lines at a certain point to make them easier to read, see newspapers and journals, because of the width of the human vision.

Now you have this information it is up to you if you want to make it unpleasant to look at your patches or make it a pleasant experience. I do not intend to force you to do anything.

For me, the point of being unpleasantly (or oddly) long is around 120 characters.

I do not have any interest in changing my code (or code comments) to make the github webui experience better, but I still take your point about there being a limit somewhere.

This comment does not add extra information and could be removed. The next line says the same.

The negation can be avoided by switching the statements to simplify the code:

if safe_mode:
    raise ...
else:
    self._blivet...

The negation can be avoided here as well.

Shouldn't the default be True here for safety? The documentation implies that True is the default and it is implemented via the role variables only.

Yes, it should. Good catch.

This comment seems to indicate that there is still something that needs to be done but to me it is unclear what is meant.

@pcahyna can you answer this one?

It is a workaround for a silly behavior in Ansible. The problem is, ansible_failed_task.name references the storage_provider variable which is not defined anymore at this point. But variables are evaluated lazily, so by evaluating ansible_failed_task.name here one crashes Ansible with an undefined variable reference. It should be done better, but I don't know how. (True, the comment should describe the problem better so it is not lost.)

here are references to Ansible bug reports: ansible/ansible#49942 ansible/ansible#57399

@tyll For the record, #88 avoids the problem. linux-system-roles/network#200 shows a different approach.

This commit does not seem to fit in this PR and it seems that the added functionality is not used or tested. Not sure what it is doing here.

Sure, I can move this out.